Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make dialog region focusable via keyboard #2428

Merged
merged 4 commits into from
Dec 18, 2023
Merged

Make dialog region focusable via keyboard #2428

merged 4 commits into from
Dec 18, 2023

Conversation

strackoverflow
Copy link
Member

@strackoverflow strackoverflow commented Dec 6, 2023 •
edited
Loading

Authors: Please fill out this form carefully and completely.

Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans.
Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.

What are you trying to accomplish?

This PR adds the ability to use the keyboard to navigate to a Dialog's scrollable body, which is currently inaccessible to keyboard users.

When a dialog body contains overflown content, the newly added <scrollable-region> element will automatically add the necessary HTML attributes to make the region focusable and screen-reader friendly.

This is a very similar approach that I used to fix the same issue in the React version of the Dialog component.

Screenshots

screenshot of focused dialog body

Integration

List the issues that this change affects.

Closes https://github.com/github/accessibility-audits/issues/5453 (GitHub staff only)
Additional context: https://github.com/github/primer/issues/2698 (GitHub staff only)

Risk Assessment

  • Low risk the change is small, highly observable, and easily rolled back.
  • Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
  • High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

Anything you want to highlight for special attention from reviewers?

Accessibility

  • Fixes axe scan violation - This change fixes an existing axe scan violation.

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews (Lookbook)
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@strackoverflow strackoverflow self-assigned this Dec 6, 2023
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 6, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 183d574

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added javascript Pull requests that update Javascript code ruby Pull requests that update Ruby code labels Dec 6, 2023
@github-actions github-actions bot added patch release and removed javascript Pull requests that update Javascript code ruby Pull requests that update Ruby code labels Dec 12, 2023
@strackoverflow strackoverflow marked this pull request as ready for review December 12, 2023 23:54
@strackoverflow strackoverflow requested review from a team and camertron December 12, 2023 23:54
@jonrohan
Copy link
Member

Looks good, would it be easy to add a system test to load the long_text preview?

camertron
camertron approved these changes Dec 15, 2023
View reviewed changes
Copy link
Contributor

@camertron camertron left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving for now, but @jonrohan is right, let's add a test!

@strackoverflow
Copy link
Member Author

strackoverflow commented Dec 15, 2023 •
edited
Loading

@jonrohan & @camertron - thanks for the suggestion, I added that system test!

I decided to write a full test asserting the focus behavior, as it seemed better to me than just loading the preview and relying on the Axe scan to detect failures, but if you think this test could become too brittle or flakey (I don't love sending the tab key two times, for example), I'd be happy to just load the preview and let the Axe scan do its thing.

What do you think?

Edit: okay the system test is already failing in the CI suite, but was passing in my Codespace. Maybe it would be better to just run the preview and let Axe do the scanning.

@camertron
Copy link
Contributor

okay the system test is already failing in the CI suite, but was passing in my Codespace. Maybe it would be better to just run the preview and let Axe do the scanning.

Yeah I'm ok with that for now. We can always improve the system test later if there are any regressions.

@camertron camertron merged commit 4cb9a57 into main Dec 18, 2023
30 checks passed
@camertron camertron deleted the dialog-scroll-region branch December 18, 2023 18:53
@primer primer bot mentioned this pull request Dec 18, 2023
Merged
@openprojectci openprojectci mentioned this pull request Jan 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camertron camertron camertron approved these changes

Assignees

@strackoverflow strackoverflow

Labels
patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@strackoverflow @jonrohan @camertron